Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add option to the protoc code generator to avoid generating deprecated code #3089

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add option to the protoc code generator to avoid generating deprecated code #3089

wants to merge 10 commits into from

Conversation

mgodave
Copy link
Contributor

@mgodave mgodave commented Nov 6, 2024 •
edited
Loading

Motivation

The generated ServiceTalk gRPC stubs make use of deprecated calls. This causes a problem when -Werror is used to build the code since it will automatically fail the build. We should allow users who have already migrated their code to prevent the protoc compiler from generating and using deprecated references.

Modifications

Add an option, skipDeprecated, as part of the protoc compiler configuration, to tell the generator to leave out deprecated references.

Result

Example: Tester.java before/after https://gist.github.com/mgodave/5772193156ca740ea90ca11bafb3da56

Remove

  • initSerializationProvider, reason: ContentCodec is deprecated
  • isSupportedMessageCodingsEmpty, reason ContentCodec is deprecated
  • Deprecated ServiceFactory constructors
  • ServiceFactory::Builder references to ContentCodec
  • Generated client metadata and associated methods, reason: deprecated

Testing

Manually tested:

  • Add new option to ServiceTalk/examples/grpc/protoc-options and use it to generate test_service.proto which covers all combinations of streaming (or not) services.

@mgodave mgodave marked this pull request as ready for review November 7, 2024 15:10
bryce-anderson
bryce-anderson approved these changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Token approval: this looks fine to me but please wait for a more experienced reviewer before merging.

How was it tested? I presume manually, but it would be good to document it in the PR description.

@mgodave
Copy link
Contributor Author

mgodave commented Nov 14, 2024

New diff from test_service.proto: https://gist.github.com/mgodave/5772193156ca740ea90ca11bafb3da56

idelpivnitskiy
idelpivnitskiy reviewed Nov 15, 2024
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good progress! New generated code looks much better than without skipDeprecated option.
Just a few adjustments for the final findings and we are ready to ship:

servicetalk-examples/grpc/protoc-options/src/main/proto/test_service.proto

package grpc.netty;

service Tester {
Copy link
Member

@idelpivnitskiy idelpivnitskiy Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was handy to use this file for validation of all API variants, but we should not add it to this example if we don't use it. Let's remove for now and we can reconsider the approach for the future similar to what you discussed with Bryce on how to observe the differences in generated code as part of PR review (follow-up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was handy for testing I feel like we should leave it or add another project just for this. I'm happy to add a usage here but it seems kind of counterproductive to have to copy/paste something in here every time we want to generate a comprehensive example for testing/comparison.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove it at the end before merging PR. It just doesn't fit into this example. And creating another module is a bit out of scope of this PR, we will plan that for the follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

servicetalk-examples/grpc/protoc-options/build.gradle Show resolved Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRoutes.java Outdated Show resolved Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcRoutes.java Outdated Show resolved Hide resolved
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java Show resolved Hide resolved
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java Show resolved Hide resolved
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java Show resolved Hide resolved
idelpivnitskiy
idelpivnitskiy approved these changes Nov 16, 2024
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

The generated code with skipDeprecated=true looks good. I also checked diff for default behavior. Many things changed their order, which is ok. Couldn't spot anything suspicious, but would be great if someone can have another look at the diff @bryce-anderson. We need to make sure current users won't be affected.

servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/Generator.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelpivnitskiy idelpivnitskiy idelpivnitskiy approved these changes

@bryce-anderson bryce-anderson Awaiting requested review from bryce-anderson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mgodave @bryce-anderson @idelpivnitskiy